Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2-quell-the-fracus] [Bug Fix] - Ensure that users hand typing tex into Numeric Inputs on Desktop do not cause an infinite loop. #2175

Closed

Conversation

SonicScrewdriver
Copy link
Contributor

@SonicScrewdriver SonicScrewdriver commented Jan 30, 2025

Summary:

This ticket is part of the Numeric Input project.

Numeric Input has different experiences on Desktop versus Mobile:

  1. Desktop allows users to enter basic math commands using their regular keyboard. Desktop does not display / visually support TeX, but CAN parse it as an answer.
  2. Mobile allows users to enter basic math commands using a keypad that crafts TeX to display the output using MathQuill / Math Input.

This diverged experience resulted in the possibility to hit an infinite loop on Desktop if the user tries to hand type \frac or \dfrac TeX commands , as the parser was unable to locate the next symbols to parse. This has likely been a bug since inception, but has become far more noticeable as we're now parsing answers on the fly to provide AI support. As a result, the answers are constantly being evaluated and would hit the infinite loop as soon as the user started typing the expressions.

Issue: LEMS-198

Test plan:

  • Run tests
  • New tex wrangler test

SonicScrewdriver and others added 8 commits January 30, 2025 10:27
We have a lot of old patterns in Perseus, and we would like to start to updating to more modern methods. This PR updates the Numeric Input logic in the following ways:

1. Moves all UI Related Numeric Input logic to a functional component, and creates a new `numeric-input.class.tsx` file for housing the static / class methods.
2. Removes all string refs related to Numeric Input in both the InputWithExamples, SimpleKeypadInput, and Tooltip components
3. Adds a little more specificity to method parameters

Issue: LEMS-2443

- Manual Testing
- Automated Tests
- Landing onto a feature branch for future QA Regression pass

Author: SonicScrewdriver

Reviewers: SonicScrewdriver, mark-fitzgerald

Required Reviewers:

Approved By: mark-fitzgerald

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x)

Pull Request URL: #2108
This PR is part of the Numeric Input Project, and will be landed onto the feature branch for a full QA pass.

The intended goal was to remove all cases of underscore in the Numeric Input component, and to improve the commenting / documentation of the code.

Some things to note:
- I've changed the logic of `generateExamples` to match `shouldShowExamples`, as we were generating a list of all possible examples and simply not displaying it. This seemed unnecessary and we can exit both functions early.
- I've added more specific types for `PerseusNumericInputWidgetOptions.simplify`

Issue: LEMS-2446

- New tests
- Manual testing in storybook

Author: SonicScrewdriver

Reviewers: SonicScrewdriver, mark-fitzgerald, jeremywiebe

Required Reviewers:

Approved By: mark-fitzgerald

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x)

Pull Request URL: #2128
…er (#2121)

## Summary:
This PR is part of the Numeric Input Project work. It is being landed onto the `feature/numeric-dx-refactor` branch. 

This PR contains the following changes:
1. Moves the InputWithExamples component to the NumericInput folder
2. Modernizes InputWithExamples to be a functional component 
3. Addition of some comments 

Video Example of Snapshot Testing:

https://github.com/user-attachments/assets/ca917778-50b0-46d2-89d8-dad95d1dadf2



Issue: LEMS-2785

## Test plan:
- Ensure all tests pass 
- Manual testing with PR Snapshot in upstream consumer 
- Landing onto feature branch that will see full QA regression pass before deployment

Author: SonicScrewdriver

Reviewers: SonicScrewdriver, mark-fitzgerald

Required Reviewers:

Approved By: mark-fitzgerald

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x)

Pull Request URL: #2121
## Summary:
This PR is part of the Numeric Input Project. 

The purpose of this PR is to improve our storybook setup for our Numeric Input Widget. This includes the following work:
- Modernizing story structure 
- Hooking up argTypes with descriptions 
- Updating RendererWithDebugUI to also set customKeypad to the same value as isMobile 
     - This allows us to ensure that our Widgets that use MathInput are properly updating when toggled into Mobile view
- Adjustments to SideBySide 
     - Automatically collapse the Perseus JSON view. 
     - Moved the PerseusJSON view below the Renderer View
     - Rename to SplitView to better encapsulate the new design 
     - Updated variable names to match

[Current (Live) Storybook Example](https://khan.github.io/perseus/?path=/docs/perseus-widgets-numericinput--docs) | [PR Storybook Example](https://650db21c3f5d1b2f13c02952-osexoxinde.chromatic.com/?path=/docs/perseus-widgets-numeric-input--docs)

Issue: LEMS-2449

## Video Example:
https://github.com/user-attachments/assets/69f6dbfb-1fda-445b-a06f-90a178f9dbeb

## Test plan:
- Ensure all tests pass + manual testing

Author: SonicScrewdriver

Reviewers: SonicScrewdriver, mark-fitzgerald

Required Reviewers:

Approved By: mark-fitzgerald

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x)

Pull Request URL: #2138
)

## Summary:
This PR is part of the Numeric Input Project. 

This is a bug fix that ensures that the Examples Tooltip for the Numeric Input widget only displays examples from the _correct_ answers. 

Issue: LEMS-2803

## Test plan:
- Run tests 
- Creation of new snapshot test that verifies wrong answerFormats are not being provided.

Author: SonicScrewdriver

Reviewers: SonicScrewdriver, handeyeco, mark-fitzgerald

Required Reviewers:

Approved By: handeyeco, mark-fitzgerald

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x)

Pull Request URL: #2159
…to Numeric Inputs on Desktop do not cause an infinite loop.

I believe this solves the issue, but I'm putting this in a PR to perform some additional testing downstream.

Issue: LEMS-198

Test Plan:
- Run tests
- New tex wrangler test
…reate infinite loop with incomplete tex in Numeric Input
@SonicScrewdriver SonicScrewdriver self-assigned this Jan 30, 2025
Copy link
Contributor

github-actions bot commented Jan 30, 2025

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (ff226d9) and published it to npm. You
can install it using the tag PR2175.

Example:

yarn add @khanacademy/perseus@PR2175

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR2175

Copy link
Contributor

github-actions bot commented Jan 30, 2025

Size Change: +112 B (+0.01%)

Total Size: 1.48 MB

Filename Size Change
packages/perseus-score/dist/es/index.js 113 kB +112 B (+0.1%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 39 kB
packages/keypad-context/dist/es/index.js 760 B
packages/kmath/dist/es/index.js 86.8 kB
packages/math-input/dist/es/index.js 77.7 kB
packages/math-input/dist/es/strings.js 1.79 kB
packages/perseus-core/dist/es/index.js 43.5 kB
packages/perseus-editor/dist/es/index.js 688 kB
packages/perseus-linter/dist/es/index.js 22.2 kB
packages/perseus/dist/es/index.js 382 kB
packages/perseus/dist/es/strings.js 5.82 kB
packages/pure-markdown/dist/es/index.js 3.66 kB
packages/simple-markdown/dist/es/index.js 12.5 kB

compressed-size-action

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant